feat: introduce owner rotation for Safe account#90
Conversation
paolodamico
requested review from
andy-t-wang,
aurel-fr,
lukejmann,
marioradonic,
murph and
tomislavhoman
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces owner rotation functionality for Safe smart accounts by implementing a new tx_swap_safe_owner method that enables key rotation through the Safe contract's swapOwner function. The implementation includes comprehensive integration tests and refactors existing code for better reusability.
- Adds Safe owner swap functionality with
SafeOwnertransaction type andtx_swap_safe_ownermethod - Refactors
Is4337Encodabletrait to provide sharedas_execute_user_op_call_dataimplementation - Renames
transaction_transfertotx_transferfor consistency and brevity
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| bedrock/src/transaction/mod.rs | Exports SafeOwner and adds tx_swap_safe_owner method to SafeSmartAccount |
| bedrock/src/transaction/contracts/safe_owner.rs | Implements SafeOwner struct and Is4337Encodable trait for owner swap functionality |
| bedrock/src/transaction/contracts/mod.rs | Adds safe_owner module export |
| bedrock/src/transaction/contracts/erc20.rs | Refactors to use new Is4337Encodable trait methods |
| bedrock/src/smart_account/transaction_4337.rs | Refactors Is4337Encodable trait with shared implementation |
| bedrock/src/smart_account/nonce.rs | Adds SwapOwner transaction type ID |
| bedrock/tests/test_smart_account_safe_owner_swap.rs | Comprehensive integration test for Safe owner swap functionality |
| bedrock/tests/common.rs | Adds AnvilBackedHttpClient mock for testing 4337 operations |
| bedrock/tests/test_smart_account_transfer.rs | Updates method name from transaction_transfer to tx_transfer |
| sol! { | ||
| ///Owner Manager Interface for the Safe | ||
| /// | ||
| /// Reference: <https://github.com/safe-global/safe-smart-account/blob/v1.4.1/contracts/base/OwnerManager.sol> |
There was a problem hiding this comment.
I'm assuming there is no difference here between 1.3.0 and 1.4.1 OwnerManager? Just wanted to ask
|
|
||
| /// Represents a Safe owner swap transaction for key rotation. | ||
| pub struct SafeOwner { | ||
| /// The inner call data for the ERC-20 `transferCall` function. |
| /// - `new_owner`: The new owner to replace the old owner | ||
| #[must_use] | ||
| pub fn new( | ||
| wallet_address: Address, |
There was a problem hiding this comment.
lets use safe_address, wallet_address is unclear
| }, | ||
| }; | ||
|
|
||
| const SENTINEL_ADDRESS: Address = |
There was a problem hiding this comment.
Can you add a doc here on what this magic address is
| ) -> Self { | ||
| Self { | ||
| call_data: IOwnerManager::swapOwnerCall { | ||
| prev_owner: SENTINEL_ADDRESS, |
There was a problem hiding this comment.
is this always the case? I'm not familiar with this logic in the contract
What if a user rotates for the 2nd/3rd time
| } | ||
| .abi_encode() | ||
| .into() | ||
| fn target_address(&self) -> Address { |
There was a problem hiding this comment.
Better to define these fns on the Is4337Encodable instead of duplicating inside each impl?
| /// - Will throw an RPC error if the transaction submission fails. | ||
| /// - Will throw an error if the global HTTP client has not been initialized. | ||
| pub async fn transaction_transfer( | ||
| pub async fn tx_transfer( |
There was a problem hiding this comment.
nit: lets remove tx_ prefix? Its already on the safe_account object
| /// # Errors | ||
| /// - Will throw a parsing error if any of the provided attributes are invalid. | ||
| /// - Will throw an RPC error if the transaction submission fails. | ||
| pub async fn tx_swap_safe_owner( |
| /// This is used to allow key rotation. The EOA signer that can act on behalf of the Safe is rotated. | ||
| /// | ||
| /// # Arguments | ||
| /// - `old_owner`: The EOA of the old owner (address). |
There was a problem hiding this comment.
we can fetch this from on-chain instead of having to take in as input
There was a problem hiding this comment.
do we need anything to enforce that this only done for world app safes? Ie check that threshold/signers length is 1
|
|
||
| // Create SafeSmartAccount instance | ||
| let safe_account = | ||
| SafeSmartAccount::new(initial_owner_key_hex, &safe_address.to_string()) |
There was a problem hiding this comment.
Can we make the parameter be Address instead of hex?
* refactors * improvements * clarify for tests
| prev_owner: SENTINEL_ADDRESS, | ||
| old_owner, | ||
| new_owner, | ||
| } |
There was a problem hiding this comment.
Bug: Incorrect prev_owner Handling in SafeOwner::new
The SafeOwner::new function hardcodes prev_owner to SENTINEL_ADDRESS (0x1) for the swapOwner call. This is only correct for single-owner Safes or the first owner swap. For multi-owner Safes or subsequent rotations, prev_owner must be the actual preceding owner, causing transactions to fail on-chain.
2 participants
Changes
swapOwneron the Safe contract to rotate the owner of the Safe.as_execute_user_op_call_datain theIs4337Encodableas for types of transactions this code will be exactly the same.transaction_transfertotx_transfer. Less verbosity, especially as we introduce other types of transactions.🔜 Changes coming soon
Notes